Open
Conversation
…ent caching (same as images)
📝 WalkthroughWalkthroughGetVideoAsset in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Logic as PooledImmichFrameLogic
participant API as Immich API
participant Disk as Filesystem
Caller->>Logic: Request GetVideoAsset(id)
Logic->>Disk: compute path "id.mp4" + check DownloadImages
alt DownloadImages = true
Logic->>Disk: ensure directory exists
Disk-->>Logic: dir ready
Logic->>Disk: file exists & check age
alt file fresh
Disk-->>Logic: return file stream
Logic-->>Caller: stream (disk-backed)
else expired or missing
Logic->>API: fetch video bytes
API-->>Logic: response (body, Content-Type?)
Logic->>Disk: write bytes to id.mp4
Disk-->>Logic: file ready
Logic-->>Caller: stream (disk-backed)
end
else DownloadImages = false
Logic->>API: fetch video bytes
API-->>Logic: response (body, Content-Type?)
Logic->>Logic: default Content-Type -> "video/mp4" if missing
Logic-->>Caller: MemoryStream with bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 165-206: In GetVideoAsset (PooledImmichFrameLogic.GetVideoAsset)
move the local cache check (using _downloadLocation, fileName and
_generalSettings.RenewImagesDuration) to run before calling
_immichApi.PlayAssetVideoAsync so we short‑circuit on cache hits; then call the
API only if cache miss or expired, wrap the returned videoResponse (and its
Stream) in a using block to ensure disposal, and on API failure fall back to
returning the cached file if present; mirror the flow used in GetImageAsset to
ensure no leaked streams and proper cache-first behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 176-180: The cache-hit branch in PooledImmichFrameLogic is
returning a hardcoded content-type "video/mp4" for cached files (see filePath,
fileName and the caller that expects a tuple of (fileName, contentType,
Stream)), which can mismatch original responses; fix by persisting the original
content-type when first saving the file (e.g., write a companion metadata file
next to filePath like filePath + ".meta" or embed a suffix) and on cache hit
read that metadata and return the stored content-type instead of the hardcoded
"video/mp4"; also add a safe fallback to "video/mp4" if the metadata is missing
or unreadable and ensure any write/read logic is added where the file is created
and where the cached file is returned.
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
190-192: Consider extracting Content-Type parsing to reduce duplication.The Content-Type extraction logic is duplicated across both branches. A small helper could improve maintainability.
♻️ Optional refactor
+ private static string GetContentType(FileResponse response, string defaultType) + { + return response.Headers.ContainsKey("Content-Type") + ? response.Headers["Content-Type"].FirstOrDefault() ?? defaultType + : defaultType; + } + private async Task<(string fileName, string ContentType, Stream fileStream)> GetVideoAsset(Guid id) { // ... cache check ... using var videoResponse = await _immichApi.PlayAssetVideoAsync(id, string.Empty); if (videoResponse == null) throw new AssetNotFoundException($"Video asset {id} was not found!"); - var contentType = videoResponse.Headers.ContainsKey("Content-Type") - ? videoResponse.Headers["Content-Type"].FirstOrDefault() ?? "video/mp4" - : "video/mp4"; + var contentType = GetContentType(videoResponse, "video/mp4");Also applies to: 208-210
Collaborator
Author
|
Workflow is not running, I don't know why. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Safari/iOS video playback by buffering videos to seekable streams
Safari requires seekable streams for video range requests. Previously, non-seekable network streams from Immich caused playback failures(error code 4 - MEDIA_ERR_SRC_NOT_SUPPORTED). Now videos are either cached to disk (when DownloadImages=true) or buffered to memory to provide seekability.
Summary by CodeRabbit
Bug Fixes
Performance